Skip to content

Conversation

neelanshsahai
Copy link

@neelanshsahai neelanshsahai commented Aug 14, 2025

In this PR

  • Added dependencies for tests
  • Seperated AppModule in di directory
  • Created a AuthenticationRepository interface
  • Created a FakeAuthRepository
  • Updated MainMenuScreen to call navigateToLogin on uiState change rather than on button click after getting passed down to nested composables
  • Updated HomeViewModel to hold and update a uiState when signOut is called
  • Added HomeViewModelTest.kt file
  • Fixed spotless issues

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary of Changes

Hello @neelanshsahai, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a unit test for the HomeViewModel, enhancing the testability of the application's view models. To facilitate this, significant architectural improvements were made, including the extraction of the Dagger Hilt AppModule into its own dedicated directory and the introduction of an AuthenticationRepository interface, along with a FakeAuthRepository for testing purposes. Additionally, the MainMenuScreen's sign-out navigation logic was refined to react directly to changes in the HomeViewModel's UI state, providing a more robust and reactive user experience.

Highlights

  • Unit Testing for HomeViewModel: Added a dedicated unit test file (HomeViewModelTest.kt) to ensure the correct behavior of the HomeViewModel, specifically its sign-out functionality.
  • Repository Layer Abstraction: Introduced an AuthenticationRepository interface and updated AuthRepository to implement it, improving modularity and testability. A FakeAuthRepository was also created for isolated testing.
  • Dependency Injection Refactoring: The Dagger Hilt AppModule was moved from ShrineApplication.kt to a new di package, centralizing dependency provision and improving code organization.
  • Reactive Sign-Out Navigation: Modified MainMenuScreen to observe the HomeViewModel's uiState for sign-out status, enabling navigation to the login screen automatically when the user is signed out, rather than relying on a direct button click callback.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces unit tests for HomeViewModel, which is a great addition. The refactoring to introduce an AuthenticationRepository interface and move AppModule improves the overall architecture and testability.

My review includes a few points:

  • A potential race condition in HomeViewModel's signOut method.
  • The FakeAuthRepository used for testing has some unimplemented methods that will cause tests to fail.
  • A minor cleanup for an unused import.

Addressing these points will make the code more robust and ensure the new tests pass correctly.

Comment on lines +32 to +34
override suspend fun signOut() {
TODO("Not yet implemented")
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The signOut method is called by HomeViewModel in the test, but it's not implemented here. This will cause the test to fail with a NotImplementedError. You should provide a minimal implementation for the methods used in the test.

Suggested change
override suspend fun signOut() {
TODO("Not yet implemented")
}
override suspend fun signOut() {
// No-op for fake repository
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The signOut method is called by HomeViewModel in the test, but it's not implemented here. This will cause the test to fail with a NotImplementedError. You should provide a minimal implementation for the methods used in the test.

Comment on lines +80 to +82
override suspend fun deleteRestoreKeyFromServer(): Boolean {
TODO("Not yet implemented")
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The deleteRestoreKeyFromServer method is called by HomeViewModel in the test, but it's not implemented here. This will cause the test to fail with a NotImplementedError. You should provide a minimal implementation for the methods used in the test. Since the return value is not used in the ViewModel, you can just return true or false.

Suggested change
override suspend fun deleteRestoreKeyFromServer(): Boolean {
TODO("Not yet implemented")
}
override suspend fun deleteRestoreKeyFromServer(): Boolean {
return true
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The deleteRestoreKeyFromServer method is called by HomeViewModel in the test, but it's not implemented here. This will cause the test to fail with a NotImplementedError. You should provide a minimal implementation for the methods used in the test. Since the return value is not used in the ViewModel, you can just return true or false.

import android.os.Build
import androidx.credentials.CredentialManager
import androidx.datastore.core.DataStore
import androidx.datastore.dataStore

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

This import is unused and can be removed.

* @param dataStore The data store for storing user data.
*/
@Singleton
class AuthRepository @Inject constructor(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should rename this so it's not so similar to the interface name. "AndroidAuthRepository" perhaps?


class FakeAuthRepository : AuthenticationRepository {
override suspend fun registerUsername(username: String): Boolean {
TODO("Not yet implemented")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You've created this fake but don't actually use any of the faked methods in this PR. Can you write at least 1 test that uses one of these methods?

Comment on lines +32 to +34
override suspend fun signOut() {
TODO("Not yet implemented")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The signOut method is called by HomeViewModel in the test, but it's not implemented here. This will cause the test to fail with a NotImplementedError. You should provide a minimal implementation for the methods used in the test.

Comment on lines +80 to +82
override suspend fun deleteRestoreKeyFromServer(): Boolean {
TODO("Not yet implemented")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

high

The deleteRestoreKeyFromServer method is called by HomeViewModel in the test, but it's not implemented here. This will cause the test to fail with a NotImplementedError. You should provide a minimal implementation for the methods used in the test. Since the return value is not used in the ViewModel, you can just return true or false.

private val repository: AuthenticationRepository,
) : ViewModel() {

private val _uiState = MutableStateFlow(HomeUiState())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

curious why it defaults to isSignedIn = true

}
}
} catch(e: Exception) {
} catch (e: Exception) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be helpful to do the reformat first and land that CL. Lot of comma and space only changes in the CL.

#tioli

@Before
fun setup() {
homeViewModel = HomeViewModel(fakeAuthRepository)
Dispatchers.setMain(StandardTestDispatcher())

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed rather than runTest {}?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants